-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Implement selection triangle in project panel #702
Conversation
…r to project panel
…lus1 Conflicts: src/styles/jsTreeTheme.less
The current positioning accounts for lion scrollbars only. Additional work is needed to account for conventional scrollbars. |
|
||
/** | ||
* Within a scrolling DOMElement, creates and positions a styled selection | ||
* div to align a single selected list item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the assumptions we make about the list: it has to be a ul, the list itself has to trigger "selectionChanged" events, and anything else that's required to make this function work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Initial review complete |
*/ | ||
function sidebarList($scrollerElement, selectedClassName) { | ||
var $listElement = $scrollerElement.find("ul"), | ||
$listItem, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this var is only used in updateSelectionMarker().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Re-review finished. Also, you'll need to merge with master (I added a variable to brackets_variables.less that should go before yours). |
…lus1 Conflicts: src/styles/brackets_variables.less
|
||
// fully scroll to the selectionMarker if it's not initially in the viewport | ||
var scrollerElement = $scrollerElement.get(0), | ||
scrollerHeight = $scrollerElement.height(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be scrollerElement.clientHeight
instead, so it doesn't include the height of the bottom scrollbar. (With the current code, the item gets partially obscured by the scrollbar.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Finished re-review |
Thanks for catching those edge cases. |
Looks good, merging |
Implement selection triangle in project panel
…mozilla.org#1763 - continue (adobe#702) * tested the menu * fixed refresh temporarily * tested refresh * updated from upstream * working on RemoteEvents.js * working on RemoteEvents and removed unnecessary files * fixed conflicts from upstream * removed whitespace * modified the event of change following professor's reference * removed console.log * removed whitespace * modified initial value * fixed refresh feature * tested whitespace indicator * removed unnecessary files
@njx